Add Support Functions for Fuzzing Attached Processes and Fix a False Hang issue in attached processes #61

parikhakshat · 2022-08-19T20:38:02Z

To add support to Jackalope for fuzzing attached processes, I needed to add a helper function FindProcessId to common.cpp. The second issue I found was in the function DebugLoop. When getting coverage by attaching to a running process, the DebugLoop would always report a hang because the running process wouldn't exit. For many running processes, the program does not exit event after a testcase is sent to it, so this would result in false hangs (Ex. fuzzing a mail server).

google-cla · 2022-08-19T20:38:08Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

ifratric

Hi, thanks for submitting!

You can see the comments for the TinyInst part below (I noticed you also submitted PR to Jackalope but I want to address this one first).

ifratric · 2022-08-23T09:30:28Z

Windows/debugger.cpp

@@ -1480,7 +1480,13 @@ DebuggerStatus Debugger::DebugLoop(uint32_t timeout, bool killing)
      dbg_continue_needed = false;
    }

-    if (timeout == 0) return DEBUGGER_HANGED;
+    if (timeout == 0) {


I don't think TinyInst should report incorrect status (process exit instead of timeout). If the calling application wants to treat hang as the process exit, then that's the responsibility of the calling application.

I think this is a valid concern, but in my testing of this particular mode, there are many targets that do not exit after a testcase has been successfully inserted and processed. For example, if someone were to fuzz the windows rdp server, the server would not exit after an rdp packet is processed. This would result in a false hang detection by the fuzzer and leave the tool effectively useless for targets like these. Would it be possible to incorporate a cli flag in fuzzer.cpp of jackalope to incorporate this functionality?

I understand what you want to accomplish, my point is just that this is not the right place to implement it. Instead of TinyInst claiming the process exited when in fact it did not, it should be the function using it, e.g. TinyInstInstrumentation::Attach or TinyInstInstrumentation::Run that should adapt its behavior.

Okay, this makes sense. I will adapt these changes into the Jackalope files then.

ifratric · 2022-08-23T09:31:23Z

Windows/debugger.h

 protected:

+  DWORD processID;


This variable is unused

ifratric · 2022-08-23T09:35:53Z

common.cpp

@@ -20,6 +20,12 @@ limitations under the License.
 #include <chrono>

 #include "common.h"
+#include <windows.h>


These includes and the FindProcessId function below are Windows-specific, but TinyInst is multiplatform. Merging as is would cause build to break on other platforms (as you can see in the status of the pull request) Any Windows-specific code must be wrapped inside
#if defined(WIN32) || defined(_WIN32) || defined(__WIN32)

ifratric · 2022-08-30T08:04:31Z

debugger.cpp

@@ -0,0 +1,1917 @@
+/*


Git thinks entire debugger.cpp / debugger.h is new. What happened here? Did you change the newline convention for the entire file? In any case, please revert this as it makes the PR impossible to review.

This reverts commit 1a406fe.

…c checks

parikhakshat · 2022-08-31T01:08:29Z

Sorry about the problems with the file structure of the repository. I was trying to commit my edits to the files and I instead uploaded the file to the repository in the incorrect location. Let me know how these edits looks and then I'll start working on fixing the Jackalope implementation.

ifratric

Thanks! I did another round of review, comments below.

ifratric · 2022-08-31T08:37:01Z

Windows/debugger.cpp

@@ -1810,19 +1808,25 @@ DebuggerStatus Debugger::Continue(uint32_t timeout) {
    dbg_last_status = DEBUGGER_TARGET_START;
    return dbg_last_status;
  }
+  if (script != NULL) {


I think this is also something that should be implemented in Jackalope rather than TinyInst. IIUC "script" is used for fuzzing sample delivery, and should thus be implemented in Jackalope by subclassing the SampleDelivery class.

ifratric · 2022-08-31T08:40:46Z

Windows/debugger.cpp

-    CloseHandle(child_thread_handle);
-    child_handle = NULL;
-    child_thread_handle = NULL;
+    if (!attach_mode) {


I think these changes need to be reverted (IIUC they were related to the DEBUGGER_HANGED change). Basically I think all changes to debugger.cpp / debugger.h can be reverted at this point.

ifratric · 2022-08-31T08:41:19Z

common.cpp

@@ -20,6 +20,14 @@ limitations under the License.
 #include <chrono>

 #include "common.h"
+#if defined(WIN32) || defined(_WIN32) || defined(__WIN32)
+	#include <windows.h>


IIRC windows.h is already included from common.h so it's not needed here.

parikhakshat · 2022-09-06T01:46:18Z

I think I fixed all the code here. I added a new commit on the Jackalope repository that moves most of the code needed for fuzzing to that repository.

ifratric · 2022-09-06T08:26:11Z

Windows/debugger.h

@@ -77,6 +77,8 @@ class Debugger {
    return last_exception;
  }

+  char * script;


This variable is now unused and can be removed, right?

parikhakshat · 2022-09-06T13:35:21Z

I just pushed a commit that removed this variable. Let me know how the rest of the edits are. Thanks!

ifratric · 2022-09-06T14:04:03Z

Cool, thanks! I think TinyInst side looks good now, except you could also completely revert changes to debugger.h and debugger.cpp as the only changes there are to the spacing. I'll take a look at Jackalope side soon. I won't merge the TinyInst PR right now, but rather when they are both ready to be merged.

ifratric · 2022-09-19T09:02:35Z

common.cpp

+          return entry.th32ProcessID;
+        }
+      }
+    }


The function should return something (zero?) if the process wasn't found.

parikhakshat added 2 commits August 19, 2022 13:16

Add files via upload

48f4449

Add files via upload

8417ab3

parikhakshat mentioned this pull request Aug 19, 2022

Add Support for fuzzing by attaching to running processes on Windows. googleprojectzero/Jackalope#38

Open

ifratric reviewed Aug 23, 2022

View reviewed changes

Remove unused variable and implement windows specific checks

1a406fe

ifratric reviewed Aug 30, 2022

View reviewed changes

parikhakshat added 2 commits August 30, 2022 20:55

Revert "Remove unused variable and implement windows specific checks"

7489c6d

This reverts commit 1a406fe.

Fix debug loop, remove unused variable, and implement windows specifi…

e7fc4aa

…c checks

ifratric reviewed Aug 31, 2022

View reviewed changes

Remove some extraneous code and move it to the Jackalope repository

2fce951

Update tinyinst repository by removing extraneous code

d2f1619

ifratric reviewed Sep 6, 2022

View reviewed changes

Remove unused script variable

fade015

ifratric reviewed Sep 19, 2022

View reviewed changes

common.cpp

return entry.th32ProcessID;

}

}

}

Copy link

Collaborator

ifratric Sep 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function should return something (zero?) if the process wasn't found.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Support Functions for Fuzzing Attached Processes and Fix a False Hang issue in attached processes #61

Add Support Functions for Fuzzing Attached Processes and Fix a False Hang issue in attached processes #61

parikhakshat commented Aug 19, 2022

google-cla bot commented Aug 19, 2022

ifratric left a comment

ifratric Aug 23, 2022

parikhakshat Aug 30, 2022

ifratric Aug 30, 2022

parikhakshat Aug 31, 2022

ifratric Aug 23, 2022

ifratric Aug 23, 2022

ifratric Aug 30, 2022

parikhakshat commented Aug 31, 2022

ifratric left a comment

ifratric Aug 31, 2022

ifratric Aug 31, 2022

ifratric Aug 31, 2022

parikhakshat commented Sep 6, 2022

ifratric Sep 6, 2022 •

edited

Loading

parikhakshat commented Sep 6, 2022

ifratric commented Sep 6, 2022 •

edited

Loading

ifratric Sep 19, 2022

Add Support Functions for Fuzzing Attached Processes and Fix a False Hang issue in attached processes #61

Are you sure you want to change the base?

Add Support Functions for Fuzzing Attached Processes and Fix a False Hang issue in attached processes #61

Conversation

parikhakshat commented Aug 19, 2022

google-cla bot commented Aug 19, 2022

ifratric left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parikhakshat commented Aug 31, 2022

ifratric left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parikhakshat commented Sep 6, 2022

ifratric Sep 6, 2022 • edited Loading

Choose a reason for hiding this comment

parikhakshat commented Sep 6, 2022

ifratric commented Sep 6, 2022 • edited Loading

Choose a reason for hiding this comment

ifratric Sep 6, 2022 •

edited

Loading

ifratric commented Sep 6, 2022 •

edited

Loading